Skip to content

test: count a check and record it in one operation (#917) - #923

Open
jdatcmd wants to merge 17 commits into
mainfrom
feat/917-machine-readable-results
Open

test: count a check and record it in one operation (#917)#923
jdatcmd wants to merge 17 commits into
mainfrom
feat/917-machine-readable-results

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Closes #917 — phase 3 of #858. Stacked on #922; base is feat/916-reconcile-accounting, so review that one first.

What was wrong

Check results were prose. check, check_num and check_text printed PASS or FAIL and nothing else, so proving that a mutation reddened one named check meant grepping text. That is how every mutation proof in this repository is currently made: a person reading FAIL <name> out of a log and retyping it into a comment.

It is also how a reverted guard once reported plain green while the check count fell from 190 to 186 — the suite passed, and the only evidence anything had changed was a number nobody was comparing.

Why this is not a second emitter

A second source of truth for how many checks ran is the defect this family of issues exists to close.

lib.sh had eleven places that bumped PGC_CHECKS, each with its own outcome line beside it. Eleven chances to add a twelfth and forget the line — which is exactly what projections.sh's expect_fail did, with ten call sites, for as long as it existed, invisible because nothing reconciled the totals.

So counting a check and recording it are one operation. A helper cannot report an outcome without being counted, and cannot be counted without reporting one, because no code path does either alone. checks run: N and the N record lines are the same increment seen twice.

lib.sh:  11 sites bumping PGC_CHECKS  ->  1, inside pgc_record

The arm that holds it is structural, not behavioural: lib.sh may bump PGC_CHECKS in exactly one place, and that place must be pgc_record. That is what stops the next expect_fail from being written, rather than catching it after a year.

The record

RESULT<TAB>suite<TAB>check name<TAB>verdict<TAB>reason

Tab separated so a check name containing spaces survives. The reason carries #915's REASON_CODE, which is what makes this more than a reformat: an unrunnable check is distinguishable from a passing one without parsing prose.

A verdict pgc_record does not recognise is recorded as a FAIL, not dropped — dropping it would leave PGC_CHECKS bumped with no outcome recorded, which is the reconciliation pgc_summary already refuses.

The human lines did not move

DISPLAY is passed to pgc_record whole rather than composed inside it, and both harnesses pin the exact strings for check, check_text, check_num and check_unrunnable. 3,762 call sites, with suites, selftests and CI all grepping ^PASS and ^FAIL, is far past what a careful refactor can be trusted on.

PGC_SUITE is resolved once at load rather than per check: pgc_record runs at every one of those call sites, and a basename fork at each is 3,762 forks a suite does not need.

The runner reconciles the two artifacts

A suite's log states checks run: N and carries N records. One function produces both, so this cannot fail by drifting — but it can fail, which is why it is asserted: a suite killed mid-way, a truncated log, a helper that prints an outcome without recording it.

A log with no count at all never reached its summary. That is a different fault from a miscount and is reported as one, rather than reading as a clean reconciliation because there was nothing to compare against.

Only a suite that reached its summary has a count to reconcile — which is why this stacks on #916, for pgc_log_shows_accounting.

Evidence

selftest    exit=0  FAILs=0
            checks run: 535   accounting: 535 passed + 0 failed + 0 unrunnable = 535
            records:    535
            registered=251 | declares accounting=239, does not=12, absent=0 | sum=251

pytest      14 passed  (test_check_results_are_machine_readable.py + test_suite_accounting.py)
shellcheck  -S error -s bash test/*.sh test/selftest/*.sh   rc=0
docs_style  PASSED (9 checks)

The matrix is the load-bearing check here and it has not run yet. This change adds a line to the stdout of every check in every suite, so the two suites (PG N) jobs over all 251 suites are what proves it breaks nothing. I will report what they say rather than assume.

TESTS.md gains section 15; sections 15-17 renumbered to 16-18, with the table of contents verified against the headers programmatically (contiguous 1..18, ToC == headers).

🤖 Generated with Claude Code

https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

jdatcmd and others added 7 commits September 9, 2026 20:21
…nes (#916)

`run_all_versions.sh` printed `suites that ran: N of M` and never checked it, and
ten registered suites exit 0 having never counted a check. `pgc_classify_suite_rc`
maps rc=0 to PASS with no further question, so those ten are counted among the
suites that "ran" -- the overcount #447 added that line to stop, one level further
down.

A count cannot close this. Two errors of opposite sign cancel, and an exempt list
maintained by hand makes the count agree by construction: the check then measures
the list rather than the run. A total derived from the collect loop is worse still,
because that loop visits every registered name and any sum over it is an identity.

So membership is derived from a property each suite carries, and two readings taken
from different places are reconciled as SETS, in both directions:

  declared  the suite's own text calls pgc_summary
  observed  the suite's log carries the `accounting:` line pgc_summary prints
            before every one of its four exit paths

Neither is a number and neither is hand-maintained. A suite that stops calling
pgc_summary moves between the sets on its own, and the two directions catch
opposite mistakes: declared-but-not-accounted is a suite that died before it could
account, and accounted-but-not-declared is a stale reading of the source.

A third term is the driver's own record of suites it chose not to dispatch.
PGC_SKIP_TIMING drops four on every CI run; they declare accounting and correctly
produce none. The record is written by the branch that makes the decision, not
inferred from the log that branch forges, and a suite in both that record and the
observed set is reported as its own distinct fault.

`checks run: 251` is not what discriminates: bench_guards, docs_style and
pg_upgrade print their own. `accounting:` is produced by pgc_summary and by nothing
else in the tree.

Removal proofs, each measured rather than argued:

  strip the comment-stripping from the declaration reader
      -> "a comment mentioning pgc_summary is not a declaration" reddens
  delete the record from the skip branch
      -> "the skip branch records the suite it did not dispatch" reddens
  drop the sort before comm
      -> "the identity catches comm reading unsorted input" reddens, 20/20
  compute inputs FROM the buckets instead of the files
      -> NOTHING reddens, which is why inputs is counted by a separate route
         and why that is said out loud in both harnesses

The declaration reader's first version piped sed into `grep -q`, and this suite runs
under `set -o pipefail`. grep -q exits the moment it matches, sed takes EPIPE, and
pipefail reports the pipeline as failed even though grep matched -- so a suite that
plainly calls pgc_summary read as not declaring accounting. It is a race, so it
passed every fixture and failed only on the real population, naming the two longest
suites, analyze_function and hilbert_curve. Selftest 040 carries the same story from
#473 and #476. The fix is grep -c, which reads to EOF; a regression arm in both
harnesses pins it with a 40,000-line fixture and shows the grep -q shape still gets
it wrong there while agreeing on a short file.

Both harnesses, one implementation: the pytest half drives the shell functions out
of run_all_versions.sh rather than reimplementing them, because a Python twin would
agree with itself.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…s the shell (#916)

Both from OffgridwithJD's review of #922, and both are real.

AN ABSENT FILE IS NOT AN EXEMPT SUITE. pgc_suite_declares_accounting returned
"no" for a file that does not exist as readily as for one that does not call
pgc_summary. A registered suite whose .sh had vanished was therefore classified
exempt, and the reconciliation read clean -- a suite disappearing from the matrix,
inside the check whose whole subject is suites going missing from the accounting.

It now answers "absent", the runner counts those and fails the major, and the
real-population block in selftest 390 carries three buckets rather than folding
absent back into exempt. Measured today: 251 registered, 0 absent.

THE COMMENT STRIPPER NOW FOLLOWS THE SHELL'S RULE. `sed 's/#.*$//'` strips from
ANY hash, so a hash inside a quoted string earlier on the line would hide a
pgc_summary call after it. It now strips only a hash at line start or after
whitespace, which is what the shell treats as starting a comment.

Measured rather than assumed, over all 251 registered suites: three carry a line
holding both a hash and pgc_summary -- analyze_function, hilbert_curve and
projections -- and in every one the hash starts the line, so no suite was misread
either way. The partition is 239/12 before and after.

The shell rule still does not cover a hash after whitespace INSIDE a quoted
string, so that residual is pinned by an arm over the corpus rather than left to
be rediscovered: no registered suite may have a hash before a pgc_summary call on
the same line.

New arms in both harnesses: absent is distinguishable from exempt, a hash inside
a word does not hide the call after it, a trailing comment does not either, an
indented comment is still a comment, and every registered suite has a file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…he twelve are (#916)

All from OffgridwithJD's review, including two corrections to claims I had
already pushed.

THE CORPUS ARM I ADDED LAST COMMIT WAS INVERTED. It required a non-whitespace
character before the hash, which is a hash inside a WORD -- the shape the
stripper handles correctly -- so it flagged the safe case and was blind to the
dangerous one it was named for. Reproduced here with their control:

    psql -c "SELECT 1 # note"; pgc_summary    reader says NO, arm did not flag
    X=a#b; pgc_summary                        reader says YES, arm FLAGGED

The arm no longer restates the hazard as a second pattern. It compares the
reader's INPUT with its OUTPUT: count the call in the raw file, count it in the
stripped text, and a lower stripped count means the stripper hid a call. That
catches it for any spelling, cannot be inverted, and IS the measurement rather
than depending on one staying true.

THE READER HAD NEVER BEEN SHOWN THE PRODUCER'S OWN OUTPUT. Every log in both
harnesses was a literal, and the format string lives a third time in
pgc_summary. Three hand-written copies of one line: a wording drift in the
PRODUCER leaves both harnesses green while the reader answers "no" for every real
suite, which would redden the whole matrix on both majors having passed its own
tests. Both harnesses now run a real two-line suite and feed the reader its
actual stdout, with a reworded control so the arm can fail.

THE TWELVE, MEASURED. My prose said ten suites "never counted a check" and my
partition said twelve; both reviewers then compounded it, because a loose pattern
for sourcing lib.sh matches portlib.sh. Measured with a tight one: NONE of the
twelve sources test/lib.sh. Each defines its own check(), and ten keep no tally at
all. The number is no longer written in prose anywhere; the reconciliation prints
it at runtime.

THE OVERCOUNT THE PR OPENS BY DESCRIBING IS NOW ACTUALLY REPORTED. #922's own CI
showed "suites that ran: 242 of 251" still counting all twelve suites whose checks
the harness cannot see -- so the change described a fix it did not make. The
summary now breaks them out, from data already in hand at that point:

    suites that ran: 242 of 251 (skipped: 9, incomplete: 0)
    of those, N accounted for their checks and M did not

Also: an absent-file answer the case now handles with a loud default arm rather
than folding an unknown verdict into "does not declare"; a sentence on why the
log reader deliberately does NOT distinguish absent from negative while the
declaration reader does; a precise note that inputs == sum(buckets) cannot be
false on the DATA -- for sets the identity always holds, measured over 400 random
pairs -- and that what it guards is comm reading unsorted input; and ci.yml's
"three wall-clock suites", which is stale at four and which this change is the one
to falsify.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…s it (#486)

#923's `suites (PG 17)` went red on a pytest name that EXISTS, while PG 18 passed
the same commit:

    FAIL  every test the document names exists in the corpus:
          got [[1: test_the_partition_over_the_registered_suites_adds_up]]

The cause is the shape selftest 080 already forbids. Its membership test was
`printf '%s\n' "$ondisk" | grep -qxF "$name"`, and the selftest runs under
`set -o pipefail`: grep -q exits the moment it matches, printf takes EPIPE, and
pipefail reports the pipeline failed though the name WAS present. The name is
then recorded absent.

WHY IT SURVIVED, AND WHY IT SURFACED NOW. Selftest 080's sweep is deliberately
non-recursive and never looked inside test/selftest/ -- the directory scoping was
never a decision, it fell out of writing "$TESTDIR"/*.sh, exactly as the bench/
hole did before it. Three fragments held the forbidden shape: 350's corpus
membership test, 300's directory coverage test, 340's Makefile sweep.

At corpus size the writer is small enough to win the race on an idle machine,
which is why it passed for so long. Measured, 170 names over 400 trials:

    printf | grep -qxF     idle: 0 false absences    under load: 6
    grep -cxF <<<          idle: 0                   under load: 0

A four-way CI matrix is the loaded case, and adding names to the corpus made the
window wider. This is the third appearance of this bug class here after #473 and
#476, and the second today.

All three sites now use grep -c on a here-string, and the sweep covers
test/selftest/ with its own coverage arm, because 080 already records that a
conditionally added glob narrows silently and a file-count premise cannot see it.

THE EXEMPTION IS DERIVED, NOT LISTED. 080's own control is inside a quoted
heredoc, and so is any deliberate demonstration of the shape; a line inside one is
text being written to a file, not a pipeline the suite runs. Comment lines are
excluded for the same reason -- the rule's explanation, and the note beside each
site fixed here, necessarily spell the shape out. A filename allowlist would have
to be maintained, and this rule exists because things that must be maintained are
not.

Two defects in the exemption itself, both found by running it rather than reading
it. It printed NR where it meant FNR, so from the second file onwards it reported
line numbers from a running total and no key matched -- the neighbouring question
answered plausibly, since single-file runs agree because NR == FNR there. And it
kept heredoc state across inputs, so it now resets per file. Its own arms pin
both: the sweep sees both lines of a probe, the exemption covers the one inside
the heredoc and not the one above it.

The "did not swallow the corpus" premise is a PROPORTION rather than a guessed
ceiling. The first version used a bare 2000 and went red at 2,502 heredoc lines in
a corpus that was entirely healthy -- a hand-written number failing the way
hand-written numbers fail here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…d printf (#486)

Reported by OffgridwithJD against the previous commit: widening the DIRECTORIES
left the PRODUCER scoped to echo and printf, so every `<command> | grep -q` was
still unswept. Twenty-six sites, two of them in lib.sh and shared by every suite
that asks whether a plan is a columnar scan.

THE SCOPING REVERSAL, AND WHY. The rule's own stated principle is a reader that
exits early AND whose EXIT STATUS is the answer being read. That is
producer-independent: the writer takes EPIPE whether it is a builtin, a psql, an
ldd or an ss. Scoping to echo and printf was a narrower implementation than the
principle, justified in the file by "a pipeline out of psql or a file is a
different question" -- which is true of `| head -1` used as TEXT and false of
`| grep -q` used as a VERDICT.

THE WORST SITE IS VACUITY, NOT A FALSE RED. native_vecskip.sh's "premise: and it
is not the scalar scan" WANTS "no", so a spurious EPIPE answer makes that premise
pass for the wrong reason. It is fixed first for that reason.

NOT CLAIMED TO BE LYING TODAY. Measured, 200 trials per size on a loaded box,
match always on line one so the answer is knowably yes:

    1,892 bytes (EXPLAIN-sized)    0/200 wrong
    8,893 bytes                    1/200   <- first observed lie
   66,894 bytes                   21/40
  288,894 bytes                   40/40
  control, match on the LAST line so grep reads to EOF: 0/200 at every size

Latent, with no floor in the mechanism -- the probability rises with size rather
than crossing a threshold, which refuted a clean pipe-capacity hypothesis.
Reasoning about "small enough" is how #473, #476 and selftest 350 each survived,
so the rule sweeps rather than reasons.

`||` IS NOT A PIPE, and the first version of the widened pattern thought it was:
`[ "$rc" = 124 ] || grep -q PAT <<<"$out"` is a fallback branch reading a
here-string, with no writer process and so no EPIPE, and both fuzz suites were
flagged for it. The leading [^|] excludes it, with an arm pinning that.

My own deliberate twin in selftest 390 moved into a quoted heredoc, so the
widened sweep exempts it by the property already built rather than by a line
number -- a filename list is the thing this rule exists to avoid.

THE MATRIX IS THE VERIFICATION HERE. lib.sh's pgc_is_columnar_scan and
pgc_uses_row_fetch are shared by many suites, and eighteen suite files changed.
The selftest cannot exercise them; the two `suites (PG N)` jobs can. Local:
selftest exit 0, 553 checks, 0 failures, shellcheck rc=0 over test/, selftest/
and bench/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
(cherry picked from commit 0d49e20)
…nchor (#916)

Both reported by OffgridwithJD against 0ce14d7, and both are the shapes this
change exists to refuse, committed inside it.

THE PARTITION ARM WAS AN IDENTITY. `_reg` was incremented in the same loop body
as the three buckets, so their sum equalled it for ANY reader. Proven rather than
argued: it passes with an always-yes reader and with an always-no reader alike,
and adding the absent bucket did not change that. A total derived from the loop
that produces the buckets cannot fail, which is the note I had just re-worded two
functions away for the same reason.

The population is now counted by a SECOND ROUTE -- the runner's own --list-suites
-- so the arm fails when the classification does not see every registered suite:
a future `continue`, a read that drops a line, a list that changes between the two
reads.

And it now says what it is. It is a COVERAGE check, not a check on the reader's
correctness. The two arms below it, which require both buckets to be occupied, are
what catch a reader that answers the same way for everything. Overrating it is how
it survived as an identity.

NOTHING EXERCISED THE ^ ANCHOR. pgc_log_shows_accounting's comment claims
"anchored and fully shaped, so the word appearing in a suite's own prose cannot
satisfy it", and the prose fixture is refused by the regex SHAPE rather than by
the anchor -- so removing ^ from the reader left every arm in both harnesses
green. The indented fixtures added earlier are for the DECLARATION reader and do
not reach this one.

The distinguishing input is a well-formed accounting line that does not start its
line. Measured against a twin with the anchor removed, mutation asserted applied:

    indented line, real reader (anchored)     no
    indented line, twin reader (unanchored)   yes
    control, line-start, real reader          yes

Inert on real data -- 0 non-line-start occurrences across 246 PG17 logs and 244
PG18 -- so this closes a coverage gap rather than a live defect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
Check results were prose. `check`, `check_num` and `check_text` printed PASS or
FAIL and nothing else, so proving that a mutation reddened one NAMED check meant
grepping text -- which is how every mutation proof in this repository is currently
made, a person reading `FAIL  <name>` out of a log and retyping it.

That is also how a reverted guard once reported plain green while the check count
fell from 190 to 186: the suite passed, and the only evidence anything had changed
was a number nobody was comparing.

THE FIX IS NOT A SECOND EMITTER. A second source of truth for how many checks ran
is the defect this family of issues exists to close. lib.sh had ELEVEN places that
bumped PGC_CHECKS, each with its own outcome line beside it -- eleven chances to
add a twelfth and forget the line, which is exactly what projections.sh's
expect_fail did with ten call sites for as long as it existed.

So counting a check and recording it are ONE operation, pgc_record. A helper
cannot report an outcome without being counted, and cannot be counted without
reporting one, because no code path does either alone. `checks run: N` and the N
record lines are the same increment seen twice. Eleven sites became one, and the
arm that holds it is structural: lib.sh may bump PGC_CHECKS in exactly one place,
and that place must be pgc_record.

The record is tab separated -- suite, name, verdict, reason -- so a check name
containing spaces survives. The reason carries #915's REASON_CODE, which is what
makes this more than a reformat: an unrunnable check is distinguishable from a
passing one without parsing prose. A verdict pgc_record does not recognise is
recorded as a FAIL rather than dropped, because dropping it would leave PGC_CHECKS
bumped with no outcome recorded -- the reconciliation pgc_summary already refuses.

THE HUMAN LINES DID NOT MOVE. DISPLAY is passed to pgc_record whole rather than
composed inside it, and both harnesses pin the exact strings for check, check_text,
check_num and check_unrunnable. 3,762 call sites, with suites, selftests and CI all
grepping `^PASS` and `^FAIL`, is far past what a careful refactor can be trusted on.

PGC_SUITE is resolved once at load rather than per check: pgc_record runs at every
one of those call sites, and a basename fork at each is 3,762 forks a suite does
not need.

The runner reconciles the two artifacts per suite: a log states `checks run: N` and
carries N records. That cannot fail by drifting, since one function does both, but
it can fail -- a suite killed mid-way, a truncated log, a helper that prints an
outcome without recording it. A log with no count at all never reached its summary,
which is a different fault from a miscount and is reported as one rather than
reading as a clean reconciliation.

Stacked on #916, which supplies pgc_log_shows_accounting: only a suite that reached
its summary has a count to reconcile against.

Evidence: selftest exit 0, 535 checks, 535 records, 0 failures; 14 pytest tests;
shellcheck -S error rc=0; docs_style PASSED.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

@linuxhikerpm linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: pgc_record is not yet the single operation for check outcomes. The timing helpers still print SKIP outcomes with no record and no count.

Reviewed exact head 0d49e200cc37eb8b871e6e7eba84cab7b542a303 (stacked on #922).

The PR claims:

A helper cannot report an outcome without being counted, and cannot be counted without reporting one, because no code path does either alone.

But test/lib.sh still has these branches outside pgc_record:

check_timing() {
    if [ "${PGC_SKIP_TIMING:-0}" = 1 ]; then
        echo "SKIP  $name (PGC_SKIP_TIMING: wall-clock measurement)"
        return 0
    fi
    ...
}

check_ratio_needs_quiet_machine() {
    if [ "${PGC_SKIP_TIMING:-0}" = 1 ]; then
        echo "SKIP  $1 (PGC_SKIP_TIMING: wall-clock ratio)"
        return 0
    fi
    ...
}

Driven directly with the new library:

SKIP  t (PGC_SKIP_TIMING: wall-clock measurement)
SKIP  r (PGC_SKIP_TIMING: wall-clock ratio)
COUNT=0
RESULT lines=0

Those are named check outcomes—the comments call both functions checks—and they are exactly the non-pass state a machine-readable result stream must distinguish. Yet the record schema accepts only PASS/FAIL/UNRUN, the structural arm only counts PGC_CHECKS assignments, and pgc_reconcile_records sees perfect 0 == 0 if a suite reaches summary after only skipped timing checks.

This is not only dead API: native_cancel.sh calls check_timing, and planner_choice_quality.sh calls check_ratio_needs_quiet_machine. The matrix currently avoids the branch by not dispatching whole timing suites, but a direct PGC_SKIP_TIMING=1 run—or a future mixed suite—emits human SKIP results absent from the machine stream while the PR says that cannot happen.

Please make skipped checks a first-class recorded verdict (with a reason such as TIMING_DISABLED) and include it in summary accounting, or explicitly remove the human SKIP outcome and model these helpers through the existing unrunnable state. Add both direct arms above and require one RESULT per emitted check outcome. Also sweep pgc_require_tools_or_skip, which has another echo "SKIP ..." path; decide whether it is a check result or document why it is not.

The runner's record-count reconciliation is useful, but it cannot detect an outcome that neither increments the count nor emits a record. The structural invariant must cover outcome emitters, not only counter assignments.

@linuxhikerpm linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: pgc_record is not yet the single operation for check outcomes. The timing helpers still print SKIP outcomes with no record and no count.

Reviewed exact head 0d49e200cc37eb8b871e6e7eba84cab7b542a303 (stacked on #922).

The PR claims:

A helper cannot report an outcome without being counted, and cannot be counted without reporting one, because no code path does either alone.

But test/lib.sh still has these branches outside pgc_record:

check_timing() {
    if [ "${PGC_SKIP_TIMING:-0}" = 1 ]; then
        echo "SKIP  $name (PGC_SKIP_TIMING: wall-clock measurement)"
        return 0
    fi
    ...
}

check_ratio_needs_quiet_machine() {
    if [ "${PGC_SKIP_TIMING:-0}" = 1 ]; then
        echo "SKIP  $1 (PGC_SKIP_TIMING: wall-clock ratio)"
        return 0
    fi
    ...
}

Driven directly with the new library:

SKIP  t (PGC_SKIP_TIMING: wall-clock measurement)
SKIP  r (PGC_SKIP_TIMING: wall-clock ratio)
COUNT=0
RESULT lines=0

Those are named check outcomes—the comments call both functions checks—and they are exactly the non-pass state a machine-readable result stream must distinguish. Yet the record schema accepts only PASS/FAIL/UNRUN, the structural arm only counts PGC_CHECKS assignments, and pgc_reconcile_records sees perfect 0 == 0 if a suite reaches summary after only skipped timing checks.

This is not only dead API: native_cancel.sh calls check_timing, and planner_choice_quality.sh calls check_ratio_needs_quiet_machine. The matrix currently avoids the branch by not dispatching whole timing suites, but a direct PGC_SKIP_TIMING=1 run—or a future mixed suite—emits human SKIP results absent from the machine stream while the PR says that cannot happen.

Please make skipped checks a first-class recorded verdict (with a reason such as TIMING_DISABLED) and include it in summary accounting, or explicitly remove the human SKIP outcome and model these helpers through the existing unrunnable state. Add both direct arms above and require one RESULT per emitted check outcome. Also sweep pgc_require_tools_or_skip, which has another echo "SKIP ..." path; decide whether it is a check result or document why it is not.

The runner's record-count reconciliation is useful, but it cannot detect an outcome that neither increments the count nor emits a record. The structural invariant must cover outcome emitters, not only counter assignments.

@linuxhikerpm linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: pgc_record is not yet the single operation for check outcomes. The timing helpers still print SKIP outcomes with no record and no count.

Reviewed exact head 0d49e200cc37eb8b871e6e7eba84cab7b542a303 (stacked on #922).

The PR claims:

A helper cannot report an outcome without being counted, and cannot be counted without reporting one, because no code path does either alone.

But test/lib.sh still has these branches outside pgc_record:

check_timing() {
    if [ "${PGC_SKIP_TIMING:-0}" = 1 ]; then
        echo "SKIP  $name (PGC_SKIP_TIMING: wall-clock measurement)"
        return 0
    fi
    ...
}

check_ratio_needs_quiet_machine() {
    if [ "${PGC_SKIP_TIMING:-0}" = 1 ]; then
        echo "SKIP  $1 (PGC_SKIP_TIMING: wall-clock ratio)"
        return 0
    fi
    ...
}

Driven directly with the new library:

SKIP  t (PGC_SKIP_TIMING: wall-clock measurement)
SKIP  r (PGC_SKIP_TIMING: wall-clock ratio)
COUNT=0
RESULT lines=0

Those are named check outcomes—the comments call both functions checks—and they are exactly the non-pass state a machine-readable result stream must distinguish. Yet the record schema accepts only PASS/FAIL/UNRUN, the structural arm only counts PGC_CHECKS assignments, and pgc_reconcile_records sees perfect 0 == 0 if a suite reaches summary after only skipped timing checks.

This is not only dead API: native_cancel.sh calls check_timing, and planner_choice_quality.sh calls check_ratio_needs_quiet_machine. The matrix currently avoids the branch by not dispatching whole timing suites, but a direct PGC_SKIP_TIMING=1 run—or a future mixed suite—emits human SKIP results absent from the machine stream while the PR says that cannot happen.

Please make skipped checks a first-class recorded verdict (with a reason such as TIMING_DISABLED) and include it in summary accounting, or explicitly remove the human SKIP outcome and model these helpers through the existing unrunnable state. Add both direct arms above and require one RESULT per emitted check outcome. Also sweep pgc_require_tools_or_skip, which has another echo "SKIP ..." path; decide whether it is a check result or document why it is not.

The runner's record-count reconciliation is useful, but it cannot detect an outcome that neither increments the count nor emits a record. The structural invariant must cover outcome emitters, not only counter assignments.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Checked latest head efeae75b after the stack was rewritten. The blocker remains: the delta from reviewed 0d49e20 changes only test_suite_accounting.py and selftest 390; test/lib.sh is unchanged.

Directly on this head, PGC_SKIP_TIMING=1 still gives:

check_timing:                    human SKIP, no RESULT, PGC_CHECKS unchanged
check_ratio_needs_quiet_machine: human SKIP, no RESULT, PGC_CHECKS unchanged

So the machine-readable stream still omits named outcomes while pgc_reconcile_records sees 0 records == 0 checks. My CHANGES_REQUESTED remains current for efeae75b; it is not only a review of the previous stack head.

jdatcmd and others added 2 commits September 10, 2026 06:48
… fails (#916)

Blocking review by @linuxhikerpm, and the finding is structural and correct.

pgc_reconcile_accounting takes the DECLARED set and the OBSERVED set. Both are
derived from the suites themselves, so a registered suite in NEITHER is outside
the universe it reconciles. Driven from the function, on the head under review:

    accounting reconciliation: inputs=0 | both=0, declared only=0, accounted only=0 | sum=0
    rc=0

with the registered set holding a name the function has no argument to see. As
they put it: treating absence of a declaration as absence from the population
preserves the overcount this change is named for.

THE POPULATION IS NOW ITS OWN CHECK, and the registered set is its first input.
Every registered suite lands in exactly one of four buckets:

    accounted        its log shows it counted its checks
    not dispatched   the driver recorded that it never ran it
    known debt       named in a tracked debt file
    unaccounted      none of the above -- FAILS, by name

ACCOUNTED TAKES EITHER MECHANISM, both runtime-observable. pgc_summary's
accounting line covers 239 suites. bench_guards and docs_style keep private
counters and print their own `checks run:` without ever sourcing lib.sh, so a
reader that knew only the first would call them unaccounted, which is false.
Measured over all twelve non-declaring suites: exactly two print a runtime count,
ten print none -- which is where the ten and the twelve come from, and why they
are different numbers.

Both mechanisms are derived rather than declared, so a suite that adopts either
leaves the debt bucket on its own. That is the property that stops the debt file
becoming a permission slip.

THE DEBT FILE IS DEBT. test/suites_without_accounting.txt names the ten suites
that count nothing at runtime, generated from a measurement rather than typed. It
is tracked, so adding a name is a diff a reviewer sees -- which is why it is a
file and not a number in the environment, per #858's own constraint. A name that
starts accounting, or stops being registered, is REPORTED rather than fatal: a
gate that reddens the moment someone fixes something teaches people not to.

Failing the live population outright would redden CI ten times from the day it
lands, which is the failure mode this family exists to prevent. Recording those
ten by name makes an eleventh fail while the ten are excused, which is the
difference between a burn-down and an exemption.

Every one of the three ways out is asserted to actually let a suite out, or
"unaccounted" would be a name for "always fails" and only the debt file would be
doing any work. And inputs == sum(buckets) over the registered population CAN be
false on the data, unlike the symmetry check's identity: a name can fall outside
all four buckets.

Both harnesses, and the arm @linuxhikerpm asked for verbatim: registered={alpha},
everything else empty, must fail and name alpha.

One of my own arms was wrong on the way: it asserted the string
"accounting.registered" appeared twice, which is a fact about how many times a
variable is spelled. It now asserts the property -- that the registered file is
written from the SUITES array itself.

Evidence: selftest exit 0, 538 checks, 0 failures; 14 pytest; shellcheck rc=0
across test/, selftest/ and bench/; docs_style PASSED. The live population is the
matrix's job and it has not run yet.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…adable-results

# Conflicts:
#	test/run_all_versions.sh
@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Reviewed efeae75b. Two findings, both with a reproduction and a control. Not approving yet, and not because of the findings: this PR's base has moved twice since that head (feat/916 is now 4eca4d22), so its 12/12 describes a tree that no longer exists. I would rather approve a rebased head than leave an approval standing against one, since this repo does not dismiss stale reviews.

The design is right and I want to say so first. Eleven sites bumping PGC_CHECKS, each with its own outcome line beside it, is eleven chances to write the next expect_fail; making counting and recording one operation removes the opportunity rather than catching the result. The structural arm — lib.sh may bump PGC_CHECKS in exactly one place, and that place must be pgc_record — is the part that stops the next one being written, and it is scoped correctly to the file that owns the invariant. Verified: grep -c 'PGC_CHECKS=$((PGC_CHECKS' lib.sh is 1, and it is inside pgc_record.

I also verified the thing most likely to break silently. Every consumer of PASS/FAIL in the tree anchors at ^, so interleaving RESULT lines into the human output cannot disturb a count: the unanchored matches I found are all over source files or over verdict words (PASSED, FAILED), and run_all_versions.sh:1272 greps ^\s*FAIL, which a line beginning RESULT cannot satisfy. Likewise the grep -A/-B consumers are all reading source, not logs, so nothing that expected a detail line now gets a RESULT line instead.

1. pgc_record pays four forks per check for something bash does for free, in the function whose own comment argues against exactly that

"$(printf '%s' "$_name" | tr '\t' ' ')"
"$(printf '%s' "$_reason" | tr '\t' ' ')"

Two command substitutions, each a subshell plus a tr. Measured on an idle container, 2000 records, two runs each:

with the tr substitutions   13.759 s / 13.087 s   = 6.88 / 6.54 ms per record
with ${_name//$'\t'/ }       0.028 s /  0.027 s   = 0.014 ms per record

About 470x, and the premise is asserted rather than assumed: the two forms produce identical output including on a name that really contains tabs (has\ta\ttabhas a tab both ways). A selftest run records 465 checks, so that is roughly 3 seconds of pure fork overhead in the gate's own suite, and the tree holds 4,866 static call sites for the helpers that route through pgc_record.

What makes it worth a line rather than a shrug is that the same function hoists PGC_SUITE out of the body on this exact ground — "a basename fork at each is 3,762 forks a suite does not need" — and then adds two forks per call. ${_name//$'\t'/ } is a parameter expansion, needs no subshell, and the comment above it can keep saying what it says.

2. Selftest 320's rule and this PR's reconciliation now disagree about what a legal direct bump is

320 states the rule as: a direct write to PGC_CHECKS must record an outcome on the same line or in the lines around it, and twelve suite files satisfy it that way on their failure paths — analyze_differential, analyze_function, native_groupagg_batch, native_repack, objstore_module, objstore_stash_recovery, parquet_export_stats, pg19_vacuum_options and others bump the counter and echo their own FAIL line.

pgc_reconcile_records requires that outcome to be a RESULT line, and an echo is not one. Driven against the real lib.sh from this head:

---- honest.sh   (both checks through lib.sh)
     checks run: 2      RESULT lines: 2      reconcile rc=0
---- direct.sh   (one check through lib.sh, one direct bump + echo -- the shape the twelve use)
     checks run: 2      RESULT lines: 1      reconcile rc=1
       records=1 but the log states checks run: 2

The control is the first arm: the same reconciler on the same kind of log is rc=0 when every check routes through pgc_record.

It only fires when the suite is already failing, so it is not a false red on a healthy tree — but it is a second red whose message points at the harness's bookkeeping rather than at the suite's own failed precondition, and a reader who trusts it goes looking in the wrong place. The real fix is small and the PR already names it: pgc_pass/pgc_fail exist "so a suite-local helper need not" touch the counters, so those failure paths should call pgc_fail instead of bumping and echoing. Alternatively tighten 320's rule to require one of the recording helpers rather than "an outcome nearby" — but then the twelve need changing anyway, so it is the same work with the rule made honest.

Two things I checked and found sound, recorded so nobody re-checks them

pgc_reconcile_records cannot be fooled by a nested suite's output: every place the runner emits another log's contents prefixes it (sed 's/^/ >> /'), so an inner RESULT or checks run: line cannot be counted as the outer suite's. And the reconciliation is honest about what it is — records and checks run are the same increment seen twice, so it cannot fail by drifting, only by truncation or a kill, which is what the PR says.

On the earlier head, suites (PG 18) going green was the first real-cluster verification of the two rewritten shared helpers, which is the part the selftest could not reach. That was worth waiting for.

jdatcmd and others added 3 commits September 10, 2026 07:06
Reported by OffgridwithJD, and it is the same class I re-worded two functions
away in this very PR: the four buckets are built by successive subtraction FROM
the registered set, so their sum equals it identically. Measured, 400 random
four-set inputs: the identity fired 0 times while the real bucket findings fired
on 353 of them.

The line stays -- the house rule asks for inputs == sum(buckets) printed beside
any list-derived claim -- but the comment now says what it can catch, because the
next reader will otherwise go looking for a data case that does not exist. What
it guards is comm being fed unsorted input, which produces buckets that are not a
partition at all.

TESTS.md and the pytest docstring said the opposite -- that this identity CAN be
false on the data, unlike the symmetry check's. That was my claim and it was
wrong; both are corrected, and both now point at the arms that carry the weight:
the ones on the unaccounted bucket.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
… made the reconciler wrong (#917)

Both from OffgridwithJD's review, and the first is embarrassing in the specific
way that makes it worth writing down.

FOUR FORKS PER RECORD, IN THE FUNCTION THAT HOISTS PGC_SUITE OUT TO AVOID THEM.
pgc_record ran `$(printf ... | tr)` twice to blank tabs -- two subshells and two
tr processes -- at every one of 3,762 check sites, three lines below a comment
explaining that a basename fork per check is 3,762 forks a suite does not need.
Parameter expansion does it free. Measured on an idle box, 2,000 calls, output
identical on every input including a real tab, a leading tab and a trailing one:

    printf | tr in $( )    3.1577 ms per call
    ${var//tab/ }          0.0096 ms per call
    ratio                       331x
    across 3,762 checks    11.9 s of pure fork overhead against 36 ms

DIRECT COUNTER WRITES MADE THE RECONCILER WRONG, and the two rules disagreed.
Selftest 320 blessed a direct PGC_CHECKS bump that records an outcome nearby;
pgc_reconcile_records requires a RESULT line. Thirteen sites across ten suites
took the first path, so on a failure the reconciler added
`records=N but the log states checks run: N+1` on top of the real failure.

The fix is not to soften the reconciler. Counting a check and recording it are one
operation, which is this change's whole argument, and a direct write is a check
counted with nothing recorded -- the hole that argument cannot have. All thirteen
now call pgc_fail, which lib.sh's own header already calls "the only supported
way to add a check from outside this file". Selftest 320 gains the stronger rule
that was not satisfiable until now: no suite using lib.sh's accounting writes
PGC_CHECKS directly. bench_guards keeps its own counter under the same name and
never sources lib.sh, and is exempt by measurement rather than by name.

TWO OF MY OWN ARMS WERE WRONG ON THE WAY THROUGH, both caught by running them.

The old premise required FIVE direct writes to EXIST, which is a premise about
the corpus rather than about the sweep -- so converting the thirteen turned it red
for exactly the reason the change is for. A premise that fails when the thing it
guards is fixed is the wrong premise. It now asserts the sweep read something, and
both rules are proven on fixtures: the original flags a bump with no outcome and
allows one with an outcome, the stronger one flags that same allowed bump, and
both exempt a private counter. Two arms reading "[]" over a clean corpus are
satisfied by a sweep that classifies nothing.

And those fixtures, written out literally, made this file flag its own three
generator lines -- the same mistake selftest 080's control avoids by living in a
quoted heredoc. The bump is now assembled from the variable name.

Evidence: selftest exit 0, 581 checks, 581 records, 0 failures; 133 pytest passed
(the 35 errors are /usr/local/pg18a absent on this host, identical on main);
shellcheck rc=0 across test/, selftest/ and bench/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…#917)

Reported by OffgridwithJD, and the finding is that the four duplicate check names
I reported from #918 are not four instances -- they are a CONVENTION, and the
count grows with every part anyone writes.

The phrasing is the cause. `premise: the pytest layer is where THIS PART thinks it
is` says "this part" precisely so the sentence can be copied into any part, and
main already carries two copies of it and two of `premise: the harness library is
where this part thinks it is`. All six of the reviewer's own in-flight branches
were adding more.

SO (suite, name) IS NOT A KEY OF CHECKS. It is a key of check NAMES, and the two
differ by however many parts share a boilerplate premise. One of them going red
would mark every sharer as observed red -- a claim about a check nothing
attacked, which is exactly what #918's ledger must not make.

pgc_record now derives the part from BASH_SOURCE: the first frame that is not
lib.sh. Not a convention change, so the next part written the same way is keyed
correctly without anyone remembering, and a premise moving between parts stops
being indistinguishable from a rename.

Parameter expansion only -- no basename fork -- because this runs at every one of
3,762 call sites, which is the mistake this same function already made once.

    RESULT <TAB> suite <TAB> part <TAB> name <TAB> verdict <TAB> reason

MEASURED OVER A REAL RUN, 583 records:

    distinct (suite, name)         579   -> 4 collisions
    distinct (suite, part, name)   582   -> 1

38 distinct parts are named. The one survivor is a GENUINE duplicate --
340-the-binary-must-be-built-from asks `premise: the fixture fingerprints at all`
twice within the same part -- which is a real defect the ledger can now name
precisely instead of losing it among convention artifacts.

Evidence: selftest exit 0, 583 checks, 0 failures; pytest; shellcheck rc=0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

@linuxhikerpm linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking at exact head 5bf0a20ab28f066ebd4d737a8c5620df795684f8: the timing-helper outcome gap remains unchanged.

  • check_timing and check_ratio_needs_quiet_machine with PGC_SKIP_TIMING=1 emit two human SKIP outcomes but leave PGC_CHECKS=0 and emit zero RESULT records.
  • Enabled control emits two PASS outcomes, increments the count to 2, and records two RESULTs.
  • Exact selftest 400 passes, but removing both timing SKIP emitters also leaves it green. Breaking global RESULT emission produces 28 failures, proving the test catches covered emitters but does not cover these branches.

The new commit adds the part field and its mutation test bites, but it does not route disabled timing outcomes through pgc_record. Please make those outcomes counted and machine-readable and add disabled/enabled arms for both helpers.

One new documentation mismatch: output now has six columns (suite, part, name, verdict, reason, mutation), while test/lib.sh:992, selftest 400’s schema prose, and TESTS.md still describe only four fields. Please update those descriptions with the behavioral fix.

@jdatcmd
jdatcmd changed the base branch from feat/916-reconcile-accounting to main September 10, 2026 14:44
@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Correction to my schema sentence in the changes-requested review: I incorrectly included mutation. At this head the producer emits six TSV columns total: the RESULT marker plus five payload fields (suite, part, name, verdict, reason). The documentation mismatch remains, but the accurate fix is to document those five payload fields. There is no mutation field in #923.

The full exact-head pass also found two broader blockers:

  1. Unrecorded conditional check outcomes extend beyond timing. native_groupagg_batch.sh conditionally prints a named fold-check SKIP, native_index_projection.sh can print up to eleven named bt_index_check SKIPs, and native_parquet_schema.sh prints PyArrow check SKIPs. These branches increment neither count nor records, so reconciliation accepts their disappearance. The fix needs an explicit recorded verdict for every per-check SKIP emitter, not only the two timing helpers.

  2. The runner validates count, not record structure. pgc_reconcile_records accepts missing fields, verdict BOGUS, extra fields, and an empty check name as long as the line starts RESULT<TAB>. pgc_record strips tabs but not newlines; a name containing first\nsecond splits one logical record across lines while both the suite and reconciliation return 0. Parse and validate the exact field count, nonempty key fields, allowed verdicts/reasons, and sanitize or reject CR/LF.

The current fixtures reveal the schema weakness: their purported valid RESULT rows omit part. Also, removing the runner integration call makes the shell twin red while pytest remains 6/6; the pytest twin must exercise the same runner boundary rather than only the extracted function.

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 10, 2026
…ompt#922's new file

commandprompt#922 merged first, so main now carries test/pytest/test_suite_accounting.py, which
needs no database. This branch's membership arm asserts set equality between the
declared NO_CLUSTER list and the files an ast property says are database-free, so
an undeclared database-free file reddens it. It did, before the entry was added:

    FAILED test_the_declaration_is_exactly_the_database_free_half
    FAILED test_the_gate_runs_the_membership_decision_rather_than_only_this_file
    2 failed, 18 passed

With "test_suite_accounting.py" declared: 20 passed, the partition is 7
database-free against 6 cluster-bound, and membership_report() returns []. That is
the coupling working rather than a cost: before this branch the file would have
been skipped by the guards job in silence.

TWO CONFLICTS, both resolved deliberately.

test/selftest/350 was COMMENT-ONLY. commandprompt#922 and this branch independently fixed
_dcv_absent's EPIPE bug and made the SAME fix -- grep -cxF on a here-string --
so the code line is identical on both sides and sits outside the conflict. That
was asserted rather than eyeballed: zero non-comment lines on either side, which
is what rules out a careless resolution restoring `printf | grep -qxF`. commandprompt#922's
comment is the base because it carries the commandprompt#923 provenance, and this branch's
measurement is folded in as a second data point: 6 false absences in 400 trials at
170 names under synthetic load, and 10 in 40 in isolation. They bracket the rate
rather than disagreeing, so the comment now says it is load- AND size-dependent.

test/pytest/TESTS.md was the table of contents and the section bodies. commandprompt#922's
section 14 keeps 14 and this branch's becomes 15, with Adding a test, What this
corpus does NOT yet refuse, and Traps this corpus records shifting to 16, 17 and
18. Checked structurally rather than by reading: 18 headings, 18 contents entries,
contiguous 1..18, and every anchor matches its heading under GitHub's own rule --
which this branch's own sweep is what enforces.

Gate on the merged tree, /usr/local/pg17a, PGC_SKIP_BUILD unset:

    harness_selftest   561 passed + 0 failed + 0 unrunnable = 561, PASSED
    pytest corpus      182 passed
    the guards subset  7 files, 127 passed, with psycopg shimmed to raise on import

The last line is the job this branch adds, run under its own condition. Its control
is that the cluster-bound files still FAIL there: test_connection.py 8 errors and
test_hilbert_locality.py 18 errors under the same shim. Without that control,
"the guards passed" is equally satisfied by a harness that reaches no database at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Adversarial pass on 5bf0a20a. One finding, and it is latent rather than live — but it is the last remaining way to break this PR's central invariant, and the PR is the right place to close it.

A subshell breaks "counting and recording are one operation", and nothing forbids writing one

The invariant is that checks run: N and the N record lines are the same increment seen twice. A counter bumped inside a subshell does not survive it; a printf inside one still reaches stdout. So a piped loop prints the outcomes and loses the count. Measured with this PR's own lib.sh:

# safe: process substitution keeps the loop in this shell
while IFS= read -r n; do check "safe $n" a a; done < <(printf 'one\ntwo\n')
# dangerous: a pipe puts the loop body in a subshell
printf 'three\nfour\n' | while IFS= read -r n; do check "piped $n" a a; done
pgc_summary
checks run: 2        <- two checks vanished from the accounting
RESULT lines: 4
PASS lines:   4
pgc_reconcile_records -> rc=1
    records=4 but the log states checks run: 2

Four checks ran, four reported PASS, and the suite's own accounting saw two. That is exactly the defect this family of issues exists to close — a check that runs without being counted — arriving through the one door pgc_record cannot close from the inside.

It is not live, and I measured that rather than assuming it. Across test/*.sh and test/selftest/*.sh there are four pipe-into-loop constructs (selftest 080:163, 270:50, 270:58, 340:681) and none of them contains a check call; a sweep for a check call inside any piped loop over the whole tree returns 0. The ten done < <(...) sites are the safe form and keep the loop in the current shell.

Your reconciliation catches it, which is a point in this PR's favour. I want that on the record: without pgc_reconcile_records the two lost checks would be invisible, and with it the run goes red. So this is not an argument against the change.

Two things I would still do:

The message does not name the cause. records=4 but the log states checks run: 2 sends the reader to the harness's bookkeeping, when the cause is their own | three lines up. A developer who writes that loop loses an hour. One clause — something like "a check counted inside a subshell does not survive it; a pipe into a loop is the usual way" — turns it into a diagnosis.

Nothing forbids writing it. Selftest 080 forbids producer | grep -q because a reader that exits early under pipefail answers wrongly; this is the same shape one level up, and the sweep is the same shape too: a check call inside a pipeline or a subshell group. Closing the class here is cheaper than finding the instance later, and this PR is where the invariant is introduced. My sweep above is a starting point and it already has a zero false-positive budget over the tree.

Verified and sound, so nobody re-checks

pgc_record adds no fork: five command substitutions, all arithmetic. The part comes from BASH_SOURCE by parameter expansion, which cannot go stale the way a convention can. Every PASS/FAIL consumer in the tree anchors at start-of-line, so interleaved RESULT lines cannot disturb a count, and nested log output is always prefixed so an inner RESULT line is never counted as the outer suite's. A mutation on a green check correctly records nothing.

One note for the merge rather than the code: test_check_results_are_machine_readable.py is database-free, so once #921 is in main this PR needs "test_check_results_are_machine_readable.py" in NO_CLUSTER in its own diff, or #921's membership arm reddens. You have already agreed to carry it; I am recording it here so it is on the PR rather than only in our messages.

…#917)

Blocking review by @linuxhikerpm at the exact head, and the finding is a hole in
this change's own argument.

check_timing and check_ratio_needs_quiet_machine, under PGC_SKIP_TIMING=1,
printed a human SKIP line and returned. Driven before the fix:

    SKIP  a timing check (PGC_SKIP_TIMING: wall-clock measurement)
    SKIP  a ratio check (PGC_SKIP_TIMING: wall-clock ratio)
    -> PGC_CHECKS=0 PGC_PASSED=0 PGC_FAILED=0 PGC_UNRUN=0

Two outcomes a reader sees, invisible to the count and to the records both, in the
change whose whole argument is that those are one operation. And nothing reached
those branches: no arm in any harness mentioned either helper, so removing both
emitters left everything green -- which is how they found it.

SKIP IS A FOURTH OUTCOME, counted like the other three. `checks run:` now reports
the checks a suite ENCOUNTERED rather than the ones it managed to evaluate, and
pgc_summary reconciles four counters against that count instead of three -- the
same shape one term wider, preserving the property its own comment argues for.

IT IS DELIBERATELY NOT check_unrunnable. That third state exists for a check whose
INPUT was absent and it exits the suite INCOMPLETE. CI sets PGC_SKIP_TIMING on
every run, so routing these through it would turn every run red. A wall-clock
check deliberately not asked on a shared runner is a different thing from one that
could not be answered, and the ledger should be able to tell them apart.

THE ALL-SKIPPED SUITE WOULD HAVE REPORTED PASSED. Before the fourth counter a
skipped check left PGC_CHECKS at zero, so `if PGC_CHECKS = 0` caught that case by
accident; counting it would have made such a suite report PASSED with nothing
behind it. The condition now says what it always meant: PASSED + FAILED + UNRUN.

REMOVAL PROOF, which is the thing their finding asked for. With both emitters
deleted -- the mutation they applied, which used to leave everything green --
NINE named arms redden across both harnesses, and lib.sh restored byte-identical
(md5 684272397ed5bb8a21a8b819a3066c59 before and after).

THE DOCUMENTATION MISMATCH IS REAL AND NARROWER THAN REPORTED. The record has FIVE
columns after the RESULT marker -- suite, part, name, verdict, reason -- and
lib.sh, selftest 400 and TESTS.md all still said four, missing `part`. Fixed in
all three. There is no `mutation` column in a record; that belongs to the LEDGER
(#918), which keys on (suite, part, name) and records which mutation reddened a
check. A record is one observation, not a history, and the docs now say so.

The accounting line changing shape means both readers on main move with it -- the
two regexes in run_all_versions.sh -- along with eleven fixtures across selftest
320, selftest 390 and test_suite_accounting.py. That coupling is exactly what
#916's producer-versus-reader arm exists to catch, and it would have caught it.

Evidence: selftest exit 0, 598 checks, 598 records, 0 failures; 23 pytest;
shellcheck rc=0; docs_style PASSED.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
@jdatcmd

jdatcmd commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Your blocker is correct and it is fixed in 0a6b2d3. I drove the gap myself before accepting it:

SKIP  a timing check (PGC_SKIP_TIMING: wall-clock measurement)
SKIP  a ratio check (PGC_SKIP_TIMING: wall-clock ratio)
-> PGC_CHECKS=0 PGC_PASSED=0 PGC_FAILED=0 PGC_UNRUN=0

Two outcomes a reader sees, invisible to the count and the records both — in the change whose whole argument is that those are one operation. And you were right that nothing reached those branches: no arm in either harness mentioned either helper, which is why deleting the emitters left everything green.

SKIP is a fourth outcome

Counted like the other three, so checks run: reports the checks a suite encountered rather than the ones it managed to evaluate. pgc_summary reconciles four counters against that count instead of three — the same shape, one term wider, preserving the property its own comment argues for.

accounting: 1 passed + 0 failed + 0 unrunnable + 1 skipped = 2

The term is printed even when zero: a term that disappears when empty is one a reader cannot tell from a term that was never there.

Deliberately not check_unrunnable. That state exists for a check whose input was absent, and it exits the suite INCOMPLETE. CI sets PGC_SKIP_TIMING on every run, so routing these through it would turn every run red. A wall-clock check deliberately not asked on a shared runner is a different thing from one that could not be answered — and #918's ledger should be able to tell them apart.

One thing your finding surfaced that neither of us named

The all-skipped suite would have reported PASSED. Before the fourth counter, a skipped check left PGC_CHECKS at zero, so if PGC_CHECKS = 0 caught that case by accident. Counting it would have made such a suite report PASSED with nothing behind it. The condition now says what it always meant: PASSED + FAILED + UNRUN.

The removal proof you asked for

With both emitters deleted — the mutation you applied, which used to leave everything green — nine named arms redden across both harnesses. lib.sh restored byte-identical, md5 684272397ed5bb8a21a8b819a3066c59 before and after.

The documentation mismatch is real, and narrower than reported

The record has five columns after the RESULT marker:

RESULT <TAB> suite <TAB> part <TAB> name <TAB> verdict <TAB> reason

lib.sh, selftest 400 and TESTS.md all said four, missing part. Fixed in all three, along with the verdict set now being PASS | FAIL | UNRUN | SKIP.

There is no mutation column in a record. That one belongs to the ledger in #925, which keys on (suite, part, name) and records which mutation reddened a check. A record is one observation, not a history — and the docs now say that explicitly, since the confusion is easy to have with #925 stacked behind this.

What moved with the accounting line

Changing its shape moves both readers on main — the two regexes in run_all_versions.sh — plus eleven fixtures across selftest 320, selftest 390 and test_suite_accounting.py. That coupling is exactly what #916's producer-versus-reader arm exists to catch, and it would have caught it if I had missed one.

Evidence

selftest    exit=0  FAILs=0   598 checks   598 records
            accounting: 598 passed + 0 failed + 0 unrunnable + 0 skipped = 598
pytest      23 passed
shellcheck  rc=0    docs_style  PASSED

The matrix is the check that matters for the accounting-line change, since every suite's log now carries the new shape and #916's readers are on main. It is running.

jdatcmd and others added 2 commits September 10, 2026 09:15
…is swept (#917)

Both from OffgridwithJD, re-swept against this head rather than carried over.

THE RECONCILER CAUGHT THE DEFECT AND DESCRIBED THE BOOKKEEPING. A check inside a
piped loop runs in a SUBSHELL, so its counter bump dies there while its outcome
and its record both reach the parent's stdout. Driven:

    check "direct one"; check "direct two"
    printf 'three\nfour\n' | while IFS= read -r n; do check "piped $n" a a; done
    -> four PASS lines, four RESULT lines, PGC_CHECKS=2

pgc_reconcile_records reported `records=3 but the log states checks run: 1`, which
is true and useless: a reader who has not met this has no route from two numbers
to a pipeline. It now names the cause, and the two directions get different
causes -- more records than counted is a lost subshell, fewer is a counter bumped
without going through pgc_record.

AND THE SHAPE IS SWEPT, the way selftest 080 sweeps its cousin. Latent today:
four piped loops in the tree, none with a check inside, so the sweep reports zero
and four fixtures prove it can fire -- a rule whose only evidence is that the
corpus happens to be clean is not a rule.

THE SWEEP WAS WRONG TWICE BEFORE IT WAS RIGHT, both found by running it over the
real corpus instead of reading it.

Requiring the closing `done` to be alone on its line left the scanner inside a
loop for the rest of any file whose loop ended `done)"`, flagging every later
check: 27 hits against a true zero.

Then a loop written entirely on ONE line inside a command substitution opened a
block that never closed -- twelve of those false hits were in the sweep's own
file. So it opens only on a line that opens a loop and does not close it, and
closes on a `done` token wherever it sits. Four fixtures now pin that: a check
inside a piped loop is found, one in a process-substitution loop is not, one
after a one-line loop is not, and a piped loop with no check in it is not.

The diagnostic message is ASSEMBLED rather than written out, because spelling the
shape made the sweep flag the line that warns about it -- selftest 080's control
problem, third time tonight.

ONE NOTE, NOT A DEFECT. pgc_log_shows_any_accounting's `checks run:` alternative
answers yes to an accounting line of any shape, so it MASKS a change to that
line: a producer moving without its readers would leave the population
reconciliation green while the accounting one reddened. Two checks disagreeing
about one log is a worse signal than either failing. It cannot happen inside one
tree, so it is recorded where someone debugging a half-merge would look.

Evidence: selftest exit 0, 608 checks, 0 failures; 23 pytest; shellcheck rc=0;
docs_style PASSED.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
jd's standing rule is that no PR ships without its CHANGELOG and docs in the same
PR. `git diff --name-only main HEAD -- CHANGELOG.md` was 0 files for this branch,
which OffgridwithJD caught: between this and #918 the stack adds pgc_record, a
machine-readable RESULT format across every call site, a fourth counted outcome,
three selftest parts and two tracked data files, and none of it was recorded.

The entry covers what a reader of the release notes needs: why one counter rather
than eleven, why the record names the part and not just the suite, why a skipped
wall-clock check is counted, and what the matrix now reconciles.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at exact head dddee7fcb473723039c6cb815d4a33cbcfcc2dd6 — 12/12 checks SUCCESS, and the rollup is attached to that sha (run 34495175533 reports headSha=dddee7fc, event=pull_request), not to an intermediate commit.

I re-measured @linuxhikerpm's two blocking items rather than reading the commit subjects, because three commits landed after their 13:58 review of 5bf0a20a.

Item 1 — the timing-helper outcome gap: closed, and the new arms bite.

Both branches now route through pgc_record, which bumps PGC_CHECKS at test/lib.sh:1012 and tallies PGC_SKIPPED:

records emitted verdict checks/passed/skipped
check_timing, PGC_SKIP_TIMING=1 1 SKIP 1/0/1
check_timing, enabled 1 PASS 1/1/0
check_ratio_needs_quiet_machine, PGC_SKIP_TIMING=1 1 SKIP 1/0/1

Prove-by-removal, each skip branch reverted to the pre-fix bare echo, one at a time (the mutation asserted present before it was applied, so neither run was a silent no-op):

  • revert check_timing's branch → records 1→0, verdict SKIP→empty, counters 1/0/1→0/0/0. The three ratio arms stay green.
  • revert check_ratio_needs_quiet_machine's branch → the ratio arms move the same way, and the timing arms stay green.

One-to-one, each mutation reddening only its own helper's arms. Note which arm does not move: and its human line is unchanged reports the identical string under both mutations. That is correct and worth saying out loud — the human SKIP line was never the gap, so an arm reading it can never have closed one.

Item 2 — the column-count mismatch: closed, with one correction to the report.

The record's shape, counted from the emitter's actual output rather than from its comment (awk -F'\t' NF):

RESULT<TAB>demo<TAB>emit<TAB>a named check<TAB>PASS<TAB>because     -> NF = 6

Six tab-separated fields including the RESULT marker, so five columns after it: suite part name verdict reason. That is what all three descriptions now say — test/lib.sh:998, test/selftest/400-a-check-result-must-be-machine.sh:36, and test/pytest/TESTS.md:1171 ("tab separated, five columns after the RESULT marker"). They agree with each other and with the run.

So there is no six-versus-four mismatch left to fix. mutation is the ledger's column, not the record's, and 400:42 now states exactly that: "No mutation column: that is the LEDGER's (#918). A record is one observation." I mention it only so the next reader of this thread does not go looking for a sixth column that is deliberately absent.

I have nothing outstanding of my own: the CHANGELOG I asked for is in at dddee7fc. @linuxhikerpm's CHANGES_REQUESTED still stands formally and the dismissal is theirs to make, not mine — this approval records that I checked both of their items against the head and found them satisfied.

What I did not verify: the arms' discriminating power was measured by driving _tm/_tmh/_tmc against test/lib.sh directly, not through a full harness_selftest.sh run, which needs parts 010/020 and a live cluster. CI ran the whole suite on PG 17 and PG 18 green, so the arms pass in situ; the mutation evidence above is what shows they can fail.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

@linuxhikerpm the 080 sweep fix you delegated is up as #926, off main, with both of your asks answered there: the false-positive budget measured over test/, test/selftest/ and bench/ (6 hits before, 0 after), and a fixture for the shape joining creates rather than reveals — it can be constructed, so the comment says so.

The count dispute is settled in your favour: six, not seven. My seventh was the || operator at test/vector_agg_rescan_memory.sh:109, which is the exact false positive you had already excluded with the leading [^|] — those lines end in || and the greps read here-strings, so there is no writer and no EPIPE. Detail on #926.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Two things found after I approved, both in the test harness rather than in shipped behaviour. The first I would fix before merge, and the fix is small. Neither retracts what I verified in the approval — pgc_record does count and record the skipped timing outcomes, and the five-column description matches the emitter — but had I had the first one before approving I would have raised it first.

1. Selftest 330 evals the runner's collect loop, and two of the functions that loop calls are undefined in the scope it is evaled into

test/selftest/330-the-incomplete-path-must-run-whole.sh extracts the runner's own per-suite collect loop as text (line 166) and evals it (lines 189, 237). It evals exactly three definitions first (lines 107-109): pgc_classify_suite_rc, pgc_verdict_fails_major, pgc_tally_suite.

At this head the extracted loop is 16 lines and calls four functions:

_verdict="$(pgc_classify_suite_rc "$_rc" "$builddir/${s}.log")"   # evaled in
pgc_tally_suite "$s" "$_verdict" "$builddir/${s}.log"             # evaled in
if [ "$(pgc_log_shows_accounting "$builddir/${s}.log")" = yes ]; then   # NOT
	if ! pgc_reconcile_records "$builddir/${s}.log"; then               # NOT
		echo "    in $s"
		_rec_bad=$((_rec_bad + 1))
	fi
fi

pgc_log_shows_accounting and pgc_reconcile_records are defined only in test/run_all_versions.sh, so in 330's shell both are absent. type -t after 330's three evals reports UNDEFINED for both.

The consequence is that the first absence hides the second. The guard is a command substitution, so an undefined function yields the empty string, "" = yes is false, and the pgc_reconcile_records block never executes. #917's reconciliation — the thing this branch exists to do — is not reached by the part whose stated job is to run that loop whole.

In situ, on this head. Full harness_selftest.sh on /usr/local/pg17a, under the lock, tree asserted clean at dddee7fc:

test/selftest/330-the-incomplete-path-must-run-whole.sh: line 198: pgc_log_shows_accounting: command not found
test/selftest/330-the-incomplete-path-must-run-whole.sh: line 198: pgc_log_shows_accounting: command not found
test/selftest/330-the-incomplete-path-must-run-whole.sh: line 246: pgc_log_shows_accounting: command not found
checks run: 608
accounting: 608 passed + 0 failed + 0 unrunnable + 0 skipped = 608
harness_selftest.sh: PASSED

Three command not found lines, and the suite reports PASSED with zero failures. The part's own arms above them — "premise: the runner's collect loop was extracted, not an empty range", "the loop delegates each verdict to pgc_tally_suite" — both pass, because they are about the other half of the loop.

Control: this is new here. main's collect loop is 6 lines and calls only the two functions 330 evals into scope, so it is clean there. The block that reaches the undefined pair arrives with this branch.

Fix. Eval the two missing definitions alongside the other three, and assert each extracted chunk is non-empty before evaling it — the part already states that premise for the other three, in its own words: "being non-empty BEFORE it is evalled, because eval "" succeeds silently". The same sentence applies to the functions the chunk calls. An arm that the loop's reconciliation branch actually ran would be stronger still, since a green eval of a dead branch is what happened here.

2. The pytest mirror's two part arms cannot fail (minor)

test/pytest/test_check_results_are_machine_readable.py:96-99 asserts that the record names exactly one part, and that the field is not blank. The field is emitted as

"${_part:-${PGC_SUITE:-unknown}}"

so when the derivation produces nothing the field falls back to the suite name. It is still exactly one value, and still not blank. Both arms are satisfied by the fallback, so neither can observe the derivation being gone. Measured: with the BASH_SOURCE walk deleted, a record reads RESULT|bash|bash|a|PASS|- — part equal to suite, non-empty.

The property itself is covered, by the bash twin, and I proved that rather than assuming it. Same mutation, full harness_selftest.sh run, and exactly two arms redden — both in test/selftest/400-a-check-result-must-be-machine.sh:

FAIL  the record names the part the check was asked from: got [harness_selftest] want [400-a-check-result-must-be-machine]
FAIL  premise: and that is this fragment, not the suite: got [same] want [different]
accounting: 606 passed + 2 failed + 0 unrunnable + 0 skipped = 608
harness_selftest.sh: FAILED

Control for that mutation run: the same tree unmutated is 608 passed + 0 failed, rc 0. So the mutation reddens two arms and only those two, which is the one-to-one this branch asks for elsewhere.

So this is redundant arms in the mirror, not an uncovered property — minor, and worth a line only because a reader comparing the two files would reasonably assume the pytest pair covers what 400's pair covers. Comparing the field to the expected part name, as 400:75 does, would make them equivalent.

What I did not verify

The pytest file could not be run on this tree at all: conftest.py imports psycopg at module level, so collection fails without it installed — which is the defect #921 defers. Finding 2's vacuity is therefore established from the emitter's measured fallback and from the bash twin's behaviour, not from a pytest run. My first attempt at a standalone driver for part 400 was not a valid instrument — it failed in the control as well as the mutation, so it could not discriminate — and the numbers above come from the real harness_selftest.sh instead.

jdatcmd added a commit that referenced this pull request Sep 10, 2026
…ng (#918)

Reported by OffgridwithJD, measured on the shipped form. I wrote that the gate
"now refuses to see the ceiling raised above its previously committed value".
That was true of pgc_ledger.py and false of run_all_versions.sh:

    the runner's exact invocation, no --against    rc=0   the raise is not refused
    --against HEAD, absolute path                  rc=0   "no prior ceiling to compare"
    --against HEAD, repo-relative path             rc=1   correctly refused

THE MIDDLE LINE IS THE ONE THAT MATTERS. `git show REF:PATH` needs a
repo-relative path and the runner passes an absolute one inside a copied build
directory, so _committed_budget returned None and the gate printed a note that
READS LIKE A PASS while the ceiling it was asked to enforce went unchecked. Asked
to compare, unable to compare, is not the same as nothing to compare -- and that
is the fail-open shape this whole change is about, in the code that closes it.

So the tool resolves the path itself, through the budget's own git toplevel, and
every failure to resolve it is an ERROR. The caller no longer has to know.

WHICH REF IS NOW A DECISION RATHER THAN A DEFAULT. `--against HEAD` compares a
committed file against ITSELF: for any change already committed the working
budget and HEAD's are identical, so it catches only an uncommitted raise. The
property that matters is that a branch may not raise the ceiling relative to
MAIN. The runner prefers origin/main, falls back to HEAD, and PRINTS the fallback
and what it costs, because a silent fallback is a gate quietly enforcing less
than it claims.

THE SCRATCH-REPO ARMS WERE NECESSARY AND NOT SUFFICIENT, which is the
gate-nothing-invokes finding one level down: they proved the tool while no wired
invocation exercised it. There are now arms in the REAL tree at the REAL path --
an absolute path resolves rather than shrugs, a budget git has never seen is an
integrity failure rather than a note, and raising the tracked ceiling in place is
refused, with the file restored byte-exact.

Also in this commit: the merge of #923's base, whose CHANGELOG entry conflicted
with this one. Both entries are kept, #917 then #918, since they describe two
changes under one heading. #925 was CONFLICTING against its base, which is why no
CI had run on it.

Evidence: selftest exit 0, 678 checks, 0 failures; 145 pytest passed (the 35
errors are /usr/local/pg18a absent on this host, identical on main); shellcheck
rc=0; docs_style PASSED; git diff --check clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
jdatcmd added a commit that referenced this pull request Sep 10, 2026
…wo failure kinds are different (#918)

THE GATE CAUGHT ITS OWN BOOTSTRAP the first time it ran in CI, which is the right
outcome for a rule that could not be satisfied. From run 34503924812:

    ledger integrity failure: --against refs/remotes/origin/feat/917-machine-readable-results
    was given, but test/check_ledger_budget.txt does not exist at that ref

#925's base is #923's branch, where the budget does not exist because THIS change
adds it. So `auto` resolved the base correctly, fetched it, found no prior, failed
closed, and reddened the matrix -- and a PR introducing the file could never pass
its own gate. OffgridwithJD hit the identical failure independently in their own
clone, resolved to their fork, where the file is also absent.

THREE STATES, NOT TWO, which is the distinction this change already draws one
level down:

    the prior ref does not resolve            ERROR. Asked to compare, unable to.
    the ref resolves, file absent there       NO PRIOR. Nothing could have been
                                              raised relative to a file that did
                                              not exist.
    the ref resolves, file present            COMPARE.

The middle one is the first-landing case and it is genuinely "nothing to compare".
It is expressed as the PROPERTY -- absent at the prior -- rather than as a flag or
a date, so it clears itself: once the file is on main every future base carries
it, and there is no exemption left for anyone to forget to remove.

It is not a hole. Deleting the budget on a branch and re-adding it higher does not
reach it, because the file still exists at the prior and the comparison happens.

AND THE RUNNER COLLAPSED THE TWO FAILURE KINDS. The gate distinguishes rc=1, a
real refusal whose fix is to regenerate the ledger, from rc=2, the gate unable to
do its job at all. The runner reported both as "has a check the ledger has never
seen" -- sending the reader at a repair that cannot help, three lines below the
gate's own "new this run=0", which says the opposite. Also OffgridwithJD, from the
CI log of this branch. It now branches, and both arms still fail the major.

THE ARMS FOR IT WERE WRONG TWICE, both times in the same way. They counted
occurrences inside `grep -A6`, `-A8` and `-A12` windows, and every one broke the
moment the call site gained a comment: a window's size is a fact about formatting.
The block is now EXTRACTED and tested, as selftest 320 already does with the
runner's classifier. Then the extraction counted the block's own explanation as a
second occurrence of the sentences it was counting, so comments are stripped --
selftest 080's control problem, met in a fifth file tonight.

One arm was deleted rather than fixed: it read a variable defined ninety lines
below it, and the block that defines the variable already asserts the same thing.

Evidence: selftest exit 0, 696 checks, 0 failures; 145 pytest passed; shellcheck
rc=0; docs_style PASSED.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
…execute (#917)

Two findings OffgridwithJD posted after approving, both real, and the first is the
one worth fixing before merge.

SELFTEST 330 CLAIMS TO RUN "THE RUNNER'S OWN COLLECT LOOP". It evalled three of
the functions that loop calls and not the two #917 added -- pgc_log_shows_accounting
and pgc_reconcile_records. The guard is a COMMAND SUBSTITUTION, so the undefined
function yielded "", the reconciliation branch never ran, and the first absence
hid the second.

Measured: a real run printed `command not found` THREE TIMES and still reported
PASSED, 608 checks, 0 failures. The part testing the loop end to end was testing a
loop with a branch that could not execute.

Both are now evalled, with the same non-empty and closing-brace premises the part
already applies to the other three.

AND THE NEXT CALLER CANNOT REPEAT IT. A premise naming five would go stale the
same way, so the loop's own text is swept for the functions it invokes and every
one must be defined. A sixth caller added later fails here rather than silently
disappearing into a command substitution.

THE SECOND FINDING: the pytest mirror's two `part` arms could not fail. The field
is `${_part:-${PGC_SUITE:-unknown}}`, so deleting the derivation still yields a
non-blank single value -- "exactly one, and not blank" is satisfied by the
fallback. They now use the case the field EXISTS for: an outer script sourcing an
inner one, where the suite and the part differ. Removing the BASH_SOURCE walk
reddens them, lib.sh restored byte-identical.

My own sweep read a variable defined 87 lines below it on the first attempt, which
is the third time tonight I have written an arm against something not yet in
scope. It is placed after the extraction now.

Evidence: selftest exit 0, 610 checks, 0 failures, zero `command not found`;
pytest; shellcheck rc=0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at exact head 51b73fdb4b777e30f81a141abf4e40439be86069 — 12/12 SUCCESS, mergeStateStatus=CLEAN.

This re-approval exists because my earlier one named dddee7fc, and a review that names a commit which is no longer the head is not a review of what would merge. It was not dismissed — this repository does not dismiss stale reviews, and reviewDecision read APPROVED against the old commit the whole time the new code sat unreviewed. That is the hazard worth naming in the thread: GitHub would have allowed the merge.

Both findings I raised after the first approval are fixed, and I verified them rather than reading the diff.

Finding 1 — selftest 330's evalled loop called two undefined functions

The decisive number, from a full harness_selftest.sh on /usr/local/pg17a, tree asserted clean at this head:

command not found occurrences: 0        (3 at dddee7fc)
checks run: 610
accounting: 610 passed + 0 failed + 0 unrunnable + 0 skipped = 610
harness_selftest.sh: PASSED

pgc_log_shows_accounting and pgc_reconcile_records are extracted at 105-106 and evalled at 123-124, and the premise at 127 requires all five functions to be type -t = function.

The generalisation is the part that matters more than the fix. every function the collect loop calls is defined here passes, and it sweeps the loop's own text rather than naming five functions — so a sixth caller fails here instead of disappearing into a command substitution. A premise naming five would have gone stale exactly as the premise naming three did. That is the shape fixed rather than the instance.

Finding 2 — the pytest mirror's part arms could not fail

Measured by mutation, not read. Deleting pgc_record's BASH_SOURCE walk:

at dddee7fc (old arms)   6 passed          <- the mutation was invisible
at 51b73fdb (new arms)   1 failed, 8 passed
                         FAILED test_each_verdict_emits_one_record_carrying_its_fields

lib.sh restored byte-identical afterwards. The outer-script-sources-inner-script fixture is the right shape: it is the case the part field exists for, so the arm now fails for the reason the field exists rather than being satisfied by the ${_part:-${PGC_SUITE:-unknown}} fallback.

What I verified for the original approval, still standing

Both PGC_SKIP_TIMING branches route through pgc_record, so the skipped outcomes are counted and machine-readable; prove-by-removal reddens only that helper's own arms, one-to-one, with the human line byte-identical — which is the point, since the human line was never the gap. The record carries five columns after the marker, counted from real output, and test/lib.sh, selftest/400 and test/pytest/TESTS.md:1171 all say five.

What I did not verify

I did not re-run the PG 18 matrix locally for this head; CI's suites (PG 18) is green and that is the evidence for it. My local runs were PG 17 on an assert build.

@linuxhikerpm your two blocking items were addressed at dddee7fc and I checked both myself before approving that head — the timing-helper outcomes and the column count. Your review still stands at 5bf0a20a, so the merge gate is waiting on it.

@jdatcmd

jdatcmd commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

@linuxhikerpm — both blockers from your review at 5bf0a20a are closed at 51b73fdb. Measured, not asserted, and the tree was restored byte-identical after every mutation.

1. The timing-helper outcome gap

Both helpers now route through pgc_record, so counting and recording are the same operation for them as for everything else. Driving them directly, no subshell, so the counter survives:

DISABLED arm (PGC_SKIP_TIMING=1)
   PGC_CHECKS=2  passed=0  skipped=2  failed=0
   RESULT records: 2      human lines: 2
ENABLED  arm (PGC_SKIP_TIMING=0)
   PGC_CHECKS=2  passed=2  skipped=0  failed=0
   RESULT records: 2      human lines: 2

PGC_CHECKS == RESULT records == human lines in both arms. Previously the disabled arm was 0 / 0 / 2.

2. The removal proof you asked for

Your sharpest point was that removing both SKIP emitters left selftest 400 green. It no longer does. I reverted both to the pre-fix shape — print the human line, return 0, never touch pgc_record — asserted the mutation applied, and re-ran 400:

FAIL  a skipped timing check emits exactly one record: got [0] want [1]
FAIL  and its verdict is SKIP: got [] want [SKIP]
FAIL  and it is counted, so checks run: reports it: got [0/0/0] want [1/0/1]
FAIL  a skipped ratio check emits exactly one record: got [0] want [1]
FAIL  and its verdict is SKIP: got [] want [SKIP]
FAIL  and it is counted: got [0/0/0] want [1/0/1]
FAIL  the accounting line reconciles four outcomes against the count:
      got  [accounting: 1 passed + 0 failed + 0 unrunnable + 0 skipped = 1]
      want [accounting: 1 passed + 0 failed + 0 unrunnable + 1 skipped = 2]

400: 64 checks, 57 passed, 7 failed        (baseline: 64 checks, 64 passed, 0 failed)

Seven arms, covering both helpers in both states, plus the reconciliation. There are disabled/enabled arms for each helper as you asked, and the human line is pinned byte-identical so the fix cannot change what a reader sees.

3. The documentation mismatch

You reported output having six columns against prose describing four. At this head the record is five fields after the RESULT marker — suite, part, name, verdict, reason — and test/lib.sh now documents five. The sixth field you saw, mutation, belongs to #918 and lands in #925, not here. Measured on real output:

fields after RESULT: 5   [RESULT|...|...|a scalar timing claim|SKIP|PGC_SKIP_TIMING]
fields after RESULT: 5   [RESULT|...|...|a ratio timing claim|SKIP|PGC_SKIP_TIMING]

Your review is the reason the gap was found at all — the suite was green over two outcomes nothing counted, and the arms that now catch it exist because you said so. Could you take another look when you have a moment? Your CHANGES_REQUESTED still stands at 5bf0a20a, and I would rather you lift it than have me merge past it.

@linuxhikerpm linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 51b73fdb4b777e30f81a141abf4e40439be86069: the timing and documentation requests are fixed, but four actionable findings remain.

Verified fixed:

  • Disabled timing helpers now produce count=2, skipped=2, two RESULT records, and two human lines; enabled controls produce two counted PASS records.
  • Reverting both timing fixes reds shell by 7 checks and pytest by 2.
  • Part derivation removal now reds both twins.
  • Selftest 330 defines and exercises the collect-loop helpers.
  • Documentation consistently describes RESULT plus five payload fields.

Still blocking:

  1. pgc_reconcile_records validates only the number of lines beginning RESULT<TAB>. Missing/extra fields, verdict BOGUS, an empty name, and newline-split records all return rc=0. The newline control emitted an NF=4 record plus a continuation line and still reconciled. Parse and enforce the exact schema, allowed verdicts, nonempty key fields, and no CR/LF.

  2. Named conditional SKIPs remain outside counting/records: groupagg’s four decline checks, index projection’s eleven bt_index_check checks, and parquet schema’s six PyArrow checks. Route every per-check outcome through the same recorded operation or document/rename it as suite-level rather than a check.

  3. The pytest twin does not cover runner wiring. Removing the runner’s reconciliation call makes the shell twin fail once while pytest remains 9/9. The independent pytest test must exercise that public runner boundary without invoking or reading the shell twin.

  4. The branch conflicts with current main in CHANGELOG.md and test/pytest/TESTS.md; current CI has not exercised the resolved result.

Focused unmodified evidence: selftest 400 64/64 and pytest 23/23. The remaining failures are gaps in what those green tests establish, not regressions in the fixes above.

jdatcmd added a commit that referenced this pull request Sep 10, 2026
Two corrections from @OffgridwithJD's review, and the first one is the
document's own rule 3 catching the document.

"39 call sites" was 36 calls plus the 3 definitions. The pattern `[^_a-z]_sh(`
matches `def _sh(` as readily as a call, which is the same class of error as
`[a-z_]+\.sh` matching `sharedir` -- already written three lines above as the
thing not to do. Counted with ast now, and the entry says how, because a number
in this section has to be re-derivable or it does not belong here.

The heading said 4 python files. Three are on main; the fourth arrives with
PR #923. The entry always said so, the heading did not, and a reader who stops
at the bold line gets a count that is wrong today.

Recounted against main at aa53c1b, after #927 and #931 landed: still 3 python
files and 7 shell files. test_raises_sqlstate.py, new on main, adds neither --
it drives pytester, not the shell.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phase 3 of #858: check results are prose, so a named check cannot be cited mechanically

3 participants